Repository navigation
refactor(composition): eliminate local_trigger_access; trigger-fire access is config + identity (§4.4) - #6374
Conversation
…gger-fire access is config + identity (§4.4)
Last Bucket-1 `Local*` deployment-type leak from
docs/reborn/2026-07-17-architecture-simplification-dto-dyn-local.md §4.4.
The `ironclaw_runner::local_trigger_access` module (~1,464 LOC, libSQL +
Postgres backends) was a shadow store duplicating state that already exists:
static owners are `RebornBuildInput.owner_id`, and SSO users are the
`StoredUser` records the identity resolver persists on login. Its
`LocalTriggerAccessSource::LocalDev{Env,Sso,Run}Bootstrap` enum was a
deployment-mode-as-type leak.
Replace it with a config value and no persisted store:
- `TriggerFireAccessPolicy` on `RebornRuntimeInput` — an OR-combined list of
`TriggerFireAccessGrant`s (`StaticOwner` / `TenantMembership`), resolved at
the serve/run edge.
- Three checkers in `ironclaw_reborn_composition::trigger_fire_access`:
`StaticOwnerTriggerFireChecker` (pure comparison), `IdentityMembership
TriggerFireChecker` (membership from the identity `RebornUserDirectory`),
and `CompositeTriggerFireChecker` (OR, retryable-unavailable precedence).
- `build_reborn_runtime` builds the matching checker from the policy when the
trigger poller is enabled; the SSO arm reads the runtime's own identity
directory (fixes the pre/post-build lifecycle the standalone store dodged)
and unifies the former local-libSQL / hosted-Postgres store fork.
- `serve`/`run` set the policy; the per-login SSO trigger-access seed and the
`build_webui_auth_surface` bootstrap parameter are removed.
- Delete the module, the composition re-exports + `LocalTriggerAccessFire
Checker` + `open_local_trigger_access_store` + `open_hosted_single_tenant_
trigger_access_store`, and the now-unused `webui-user-store` /
`filesystem-local-trigger-access` Cargo features.
Deliberate, tested behavior change: SSO membership shifts from "seeded on
login" to "Active StoredUser in this tenant" — faithful (admission runs before
user creation; channel actors can't mint) and stricter (suspension now revokes
trigger-fire access, which the seed-only store never did).
Enforcement + coverage: `FROZEN_OTHER_MODE_TYPES` trimmed of the
`LocalTriggerAccess*` / `Reborn*LocalTriggerAccess*` entries; composition
pub-use snapshot regenerated; 13 checker/integration unit tests (incl. a real
`FilesystemRebornIdentityStore`-backed membership test asserting suspension
revokes); serve/run/webui_auth/user_directory tests rewritten to assert the
policy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
github-actions
Bot
added
scope: docs
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change replaces local trigger-access persistence and bootstrap wiring with configured trigger-fire policies, runtime-composed authorization checkers, and identity-directory membership checks. Feature flags, exports, tests, ratchets, and documentation are updated. ChangesTrigger-fire access migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant RebornRuntimeInput
participant build_reborn_runtime
participant TriggerFireChecker
CLI->>RebornRuntimeInput: set TriggerFireAccessPolicy
RebornRuntimeInput->>build_reborn_runtime: provide policy and optional override
build_reborn_runtime->>TriggerFireChecker: compose grant-based checker
TriggerFireChecker-->>build_reborn_runtime: effective authorization checker
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
⏭️ IronLoop Review Declined: reviewer
Review at a glance
| Disposition | Head |
|---|---|
| ⏭️ Review declined | 75703603734e |
Head: 75703603734edda850b69e6b3388b2377882d9f4
Reason: The actual comparison changes 641 files with 55,765 insertions and 29,671 deletions (including 18 binary files), spanning many unrelated crates, CI, documentation, and frontend areas. This cannot be reviewed reliably within the configured scope; it also materially exceeds the PR description's stated ~1.6k LOC change.
Next: Split the unrelated changes into separate PRs, or provide the intended intermediate base SHA containing the prerequisite work so the trigger-fire-access delta can be reviewed independently.
Run details
Status: Current
Trustworthy review produced: no
Summary
Skipped: the supplied base-to-head comparison is an oversized, cross-cutting mega-diff rather than a reviewable focused refactor.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the architecture by removing the persisted parallel local_trigger_access store and replacing it with a configuration-driven TriggerFireAccessPolicy on RebornRuntimeInput. Fire-time trigger authorization is now performed via either a pure comparison against a config-supplied static owner or a membership lookup against the canonical identity directory. The review feedback recommends avoiding the recently stabilized is_none_or method to maintain MSRV compatibility, and optimizing CompositeTriggerFireChecker to prevent cloning the request on the final iteration of the loop.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let allowed = user.is_some_and(|user| { | ||
| user.status == ironclaw_reborn_identity::RebornUserStatus::Active | ||
| && user | ||
| .tenant_id | ||
| .as_ref() | ||
| .is_none_or(|tenant| tenant == &self.tenant_id) | ||
| }); |
There was a problem hiding this comment.
Avoid using nightly or recently stabilized features like is_none_or if they are not supported by the project's MSRV. Consider using an explicit match expression instead to avoid compatibility issues and potential clippy warnings.
| let allowed = user.is_some_and(|user| { | |
| user.status == ironclaw_reborn_identity::RebornUserStatus::Active | |
| && user | |
| .tenant_id | |
| .as_ref() | |
| .is_none_or(|tenant| tenant == &self.tenant_id) | |
| }); | |
| let allowed = user.is_some_and(|user| { | |
| user.status == ironclaw_reborn_identity::RebornUserStatus::Active | |
| && match user.tenant_id.as_ref() { | |
| Some(tenant) => tenant == &self.tenant_id, | |
| None => true, | |
| } | |
| }); |
References
- Avoid using nightly Rust features in production-grade crates. If a nightly feature like
is_none_oris suggested, consider alternative stable Rust constructs like explicitmatchexpressions to avoid compatibility issues and potential clippy warnings.
| async fn check_trigger_fire_access( | ||
| &self, | ||
| request: TriggerFireAccessCheck, | ||
| ) -> Result<TriggerFireAccessDecision, TriggerFireAccessError> { | ||
| let mut unavailable: Option<TriggerFireAccessError> = None; | ||
| for checker in &self.checkers { | ||
| match checker.check_trigger_fire_access(request.clone()).await { | ||
| Ok(TriggerFireAccessDecision::Allowed) => { | ||
| return Ok(TriggerFireAccessDecision::Allowed); | ||
| } | ||
| Ok(TriggerFireAccessDecision::Denied { .. }) => {} | ||
| Err(error) => unavailable = Some(error), | ||
| } | ||
| } | ||
| match unavailable { | ||
| Some(error) => Err(error), | ||
| None => Ok(denied()), | ||
| } | ||
| } |
There was a problem hiding this comment.
In CompositeTriggerFireChecker::check_trigger_fire_access, request is cloned on every iteration of the loop. However, on the last iteration, we can consume request directly without cloning it, avoiding unnecessary heap allocations for the various IDs contained in TriggerFireAccessCheck. We can achieve this cleanly by using split_last() on the checkers slice.
async fn check_trigger_fire_access(
&self,
request: TriggerFireAccessCheck,
) -> Result<TriggerFireAccessDecision, TriggerFireAccessError> {
let mut unavailable: Option<TriggerFireAccessError> = None;
if let Some((last, rest)) = self.checkers.split_last() {
for checker in rest {
match checker.check_trigger_fire_access(request.clone()).await {
Ok(TriggerFireAccessDecision::Allowed) => {
return Ok(TriggerFireAccessDecision::Allowed);
}
Ok(TriggerFireAccessDecision::Denied { .. }) => {}
Err(error) => unavailable = Some(error),
}
}
match last.check_trigger_fire_access(request).await {
Ok(TriggerFireAccessDecision::Allowed) => {
return Ok(TriggerFireAccessDecision::Allowed);
}
Ok(TriggerFireAccessDecision::Denied { .. }) => {}
Err(error) => unavailable = Some(error),
}
}
match unavailable {
Some(error) => Err(error),
None => Ok(denied()),
}
}…ackage flags The local_trigger_access elimination removed the webui-user-store feature; the runner CI package-flags line still requested it, which would fail feature resolution. libSQL coverage stays via libsql-secrets + libsql-restart-tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🚅 Deployed to the ironclaw-pr-6374 environment in ironclaw-ci-preview
|
Main deleted `webui-v2-beta` (and 13 other compile-time features, #6296: "the beta host mounts are unconditional"). This branch was written against a pre-#6296 main and gated all its new code on `#[cfg(feature = "webui-v2-beta")]`. Reconciled by un-gating: - trigger_fire_access.rs: IdentityMembershipTriggerFireChecker (+ its impls and the identity test module) and the TenantId import are now unconditional; `ironclaw_reborn_identity` is a plain composition dep on main. - runtime.rs: the `TenantMembership` grant arm is unconditional; deleted the `#[cfg(not(feature = "webui-v2-beta"))]` dead-alternate arm (per .claude/rules/cargo-features.md). - runtime/mod.rs (cli): un-gated the policy imports and run-path wiring. - Cargo.toml: dropped the `webui-user-store` / `filesystem-local-trigger-access` runner feature forwards and the base `webui-user-store` runner dep feature; package-feature-flags.sh runner line reconciled to `libsql-secrets,libsql-restart-tests` (both stale sides corrected). - Conflicts in lib.rs / input.rs / facade_factory.rs resolved by keeping this branch's deletion of the local_trigger_access re-exports, checker, and hosted store; composition pub-use snapshot regenerated. Verified on merged tree: clippy -D warnings (composition, ironclaw, runner, default features) clean; ironclaw_architecture green (incl. pub-use snapshot); composition/runner/cli test suites pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@ironloopai review |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 2 | 0 | 2 | 6a849b76e7e4 |
Head: 6a849b76e7e4990285ea729411034afa0521ccd1
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
The new policy checkers lose tenant isolation and deny legacy SSO users whose identity migration has no StoredUser record, permanently failing their scheduled fires.
Findings
Blocking: 2 / Notes: 0
Blocking findings
1. ❌ [HIGH] Constrain trigger-fire grants to the poller tenant
Location: crates/ironclaw_reborn_composition/src/trigger_fire_access.rs:65-66
Neither new policy checker verifies request.tenant_id against the runtime tenant: this static grant only compares owner and scope, while the membership checker compares the stored user to its own tenant. list_due_triggers scans the shared repository across tenants, so a poller can authorize and execute a foreign-tenant trigger when owner and agent/project coincide. The deleted store checked the request tenant exactly. Bind the policy/authorizer to the runtime tenant and add a foreign-tenant same-owner/scope denial test.
2. ❌ [HIGH] Preserve trigger access for migrated legacy SSO users
Location: crates/ironclaw_reborn_composition/src/trigger_fire_access.rs:112
The legacy user_identities fold only writes external-identity/index records, not a StoredUser; the resolver explicitly allows an identity pointing to a missing user record. Those users can still log in, but this lookup returns None and the new membership policy denies their existing triggers. Authorization denial becomes permanent trigger materialization failure, including terminal failure for one-shot triggers. Hydrate/migrate an active StoredUser for legacy identities (or retain equivalent access) and cover a migrated identity firing an existing trigger.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| &self, | ||
| request: TriggerFireAccessCheck, | ||
| ) -> Result<TriggerFireAccessDecision, TriggerFireAccessError> { | ||
| let allowed = request.creator_user_id == self.owner |
There was a problem hiding this comment.
This policy must also reject a request.tenant_id outside the runtime tenant. The due-trigger repository is global, so owner/scope-only matching can authorize a foreign tenant's record; the previous store checked tenant as part of the exact access tuple.
| if !scope_matches(&request, &self.agent, &self.project) { | ||
| return Ok(denied()); | ||
| } | ||
| let user = self |
There was a problem hiding this comment.
Legacy user_identities migration does not create StoredUser rows, while legacy identity resolution intentionally permits a missing user record. Those users will always get None here and their existing scheduled triggers now fail permanently. Preserve/hydrate their membership before relying on this directory lookup.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.4% — 321581 / 372217 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
…nt composite clone Addresses PR #6374 review: - StaticOwnerTriggerFireChecker now rejects a foreign request.tenant_id. The due-trigger repository is global, so owner+scope-only matching could authorize another tenant's trigger whose creator id equals this owner — the former store keyed every row on tenant. Tenant is injected from the runtime scope at build. Regression test: static_owner_denies_foreign_tenant. - CompositeTriggerFireChecker passes the request to its last checker by move (split_last), dropping one clone per fire on the common StaticOwner+SsoMembership pair. - Kept is_none_or (stable 1.82; workspace is edition 2024 / Rust >=1.85) with a comment — clippy enforces it over map_or. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — addressed the automated review in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/trigger_fire_access.rs (1)
125-131: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog the bound source error before mapping.
The
map_errclosure drops the originalRebornIdentityErrorcause chain by converting it to a string. As per coding guidelines, you must preserve the source error or log the bound source before mapping when a constructor (likeUnavailable) only accepts a string representation.♻️ Proposed fix
let user = self .directory .get_user(&request.creator_user_id) .await - .map_err(|error| TriggerFireAccessError::Unavailable { - reason: error.to_string(), + .map_err(|error| { + tracing::warn!(%error, "identity directory lookup failed during trigger fire access check"); + TriggerFireAccessError::Unavailable { + reason: error.to_string(), + } })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_composition/src/trigger_fire_access.rs` around lines 125 - 131, Update the error handling around the directory lookup in the trigger-fire access flow to log the original RebornIdentityError before converting it into TriggerFireAccessError::Unavailable. Bind the source error in the map_err closure, emit it through the existing logging mechanism, and retain the current reason string and error mapping behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/trigger_fire_access.rs`:
- Around line 125-131: Update the error handling around the directory lookup in
the trigger-fire access flow to log the original RebornIdentityError before
converting it into TriggerFireAccessError::Unavailable. Bind the source error in
the map_err closure, emit it through the existing logging mechanism, and retain
the current reason string and error mapping behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a8bfa5ce-2874-42c5-9186-576a33ab528e
📒 Files selected for processing (2)
crates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/trigger_fire_access.rs
…m-goal-store (#6378) Continues the runner feature-flag cleanup after #6374 removed `local_trigger_access` (and with it `webui-user-store` / `filesystem-local-trigger-access`). Removes the two remaining flags that no shipped build shape turns off, leaving `libsql-restart-tests` as the runner's sole flag — a sanctioned CI test-lane selector with zero `src/` `#[cfg]`. libsql-secrets: - Gated `src/secrets.rs`, a libSQL `FilesystemSecretStore` assembly no shipped build enabled (only the CI compile self-test). The production assembly already lives in `ironclaw_reborn_composition::factory` (`build_secret_store` / `open_local_dev_secret_store`). - Removes the module + `tests/secrets.rs`, the feature, and the now-orphaned optional deps `ironclaw_secrets` and `secrecy`. filesystem-goal-store: - Gated `FilesystemSubagentGoalStore` + the `await_edge` submodules, isolating only `ironclaw_filesystem` (a cheap path dep). It was forwarded by composition's *both* `libsql` and `postgres` features and by product_workflow — on in every build, i.e. the product, not a build shape. - Makes `ironclaw_filesystem` an unconditional dep and de-gates the code; drops the forwards in composition (`libsql`/`postgres`) and the product_workflow dev-dep feature. De-gating also resolves the pre-existing dead-code warnings in the runner's zero-feature build (the `await_edge`/`untrusted_text` helpers are now always compiled and reachable). Behavior unchanged: both modules/paths were on in every shipped build or dead in all of them. Verified: runner tests (default), runner clippy (default + libsql-restart-tests), workspace feature matrix (default + all-features), and `cargo test -p ironclaw_architecture`. Supersedes #6377. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…y cutover, #6386 authorize() consolidation, #6408 outbound caller-scoping) Eighth fold. Dispositions follow the standing philosophy (never merge-as-is, never drop a feature); ledger entry lands in the PR body. - Tier B adopted wholesale: src/ + gateway/tui crates deleted, root package is the test-only ironclaw_reborn_integration_tests host, root Dockerfile is the Reborn image (de-migrated per owner decision D1 — this tree deletes ironclaw_reborn_migration; the D1 absence pin moves to the root Dockerfile), legacy test_rig/support files gone with their [[test]] entries. - #6386 authorize() consolidation: production side taken verbatim; the local-manifest trust test main relocated to ironclaw_capabilities::trust is re-expressed in this tree's manifest dialect (host_api sections + contracts registry arg). - #6408 outbound caller-scoping made structural on the generic lane: the OutboundDeliveryTargetOwner vocabulary + owner field are defined locally in composition (ironclaw_channel_host stays deleted), both registry paths keep main's entry_owned_by_caller filtering, and the generic channel provider stamps owners from the resolved resource (subject route / DM record user), never the caller. - #6374 trigger-fire access as config: main's TriggerFireAccessPolicy wiring and serve tests adopted; the LocalTriggerAccess* store lane stays deleted. - #6395 SSO/admin identity resolver: production-substrate branch adopted; the legacy libSQL identity fold stays out (greenfield blank-slate). - #6387 factory/facade test extraction: main's module topology adopted; this branch's test-module content three-way-merged into factory/tests.rs and webui/facade/tests.rs. - CI: package allowlist keeps this tree's crate set + main's root-package exclusion; bucket lanes for deleted crates removed (self-test repinned). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
What & why
Eliminates the last Bucket-1
Local*deployment-type leak fromdocs/reborn/2026-07-17-architecture-simplification-dto-dyn-local.md§4.4:the
ironclaw_runner::local_trigger_accessmodule (~1,464 LOC, libSQL +Postgres backends). It was a shadow store duplicating state that already
exists — static owners are
RebornBuildInput.owner_id; SSO users are theStoredUserrecords the identity resolver persists on every login — and itsLocalTriggerAccessSource::LocalDev{Env,Sso,Run}Bootstrapenum was adeployment-mode-as-type leak.
The prior
refactor/deployment-configbranch was already subsumed by #6279(DeploymentConfig + the
LocalDev*collapse) and is not used here.Approach — trigger-fire access as config, no persisted store
TriggerFireAccessPolicyonRebornRuntimeInput: an OR-combined list ofTriggerFireAccessGrant(StaticOwner{owner,agent,project}/TenantMembership{agent,project}), resolved at the serve/run edge.ironclaw_reborn_composition::trigger_fire_access:StaticOwnerTriggerFireChecker(pure comparison, no I/O),IdentityMembershipTriggerFireChecker(membership from the identityRebornUserDirectory— theStoredUserSSO login already writes), andCompositeTriggerFireChecker(OR, with retryable-Unavailableprecedence).build_reborn_runtimebuilds the matching checker from the policy whenthe trigger poller is enabled. The SSO arm reads the runtime's own identity
directory — resolving the pre/post-build lifecycle the standalone store
dodged, and unifying the former local-libSQL / hosted-Postgres store
fork into one identity-backed path.
serve/runset the policy; the per-login SSO trigger-access seed and thebuild_webui_auth_surfacebootstrap parameter are removed.LocalTriggerAccessFire Checker+open_local_trigger_access_store+open_hosted_single_tenant_trigger_access_store, and the now-unusedwebui-user-store/filesystem-local-trigger-accessCargo features.SSO membership shifts from "was seeded on login" to "has an Active
StoredUserin this tenant." This is faithful-or-stricter:resolve_or_create, andresolve_or_createrejects channel actors — so the admitted set is the same.suspended user's trigger-fire access; the identity-backed check denies them.
Pinned by
suspended_member_is_deniedand the real-identity integration test.Tests
trigger_fire_access.rs: static allow/deny/scope-mismatch;identity member/unknown/suspended/wrong-tenant/backend-error; composite
OR; and
real_identity_store_membership_backs_fire_accessdriving a realFilesystemRebornIdentityStorethroughresolve_or_create→ membership check→ suspend → deny (crate-tier because the checker is
pub(crate); an externaltests/file can't construct it).TriggerFireAccessPolicy(union preserved when poller + SSO are both on).Enforcement / scope
reborn_deployment_mode_typename_ratchet.rs:LocalTriggerAccess*/Reborn*LocalTriggerAccess*entries removed from the allowlist (the moduleis gone). Composition pub-use snapshot regenerated.
LocalInvocationServicesResolveris aBucket-2 mechanical de-prefix, not a deployment type — kept in the allowlist.
Verification
cargo fmt;cargo test -p ironclaw_reborn_composition --features webui-v2-beta;cargo test -p ironclaw_runner --features filesystem-goal-store;cargo test -p ironclaw_architecture; clippy oncomposition/cli/runner under both
webui-v2-betaand default features;cargo check --workspace;scripts/pre-commit-safety.sh(compositionmass/dispatch within budget). All green.
🤖 Generated with Claude Code